-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
data-menu: remove/deprecate old data menu #3281
Conversation
44a42dc
to
40df2dd
Compare
ae9a458
to
2afd53e
Compare
dccca7e
to
6dda0d5
Compare
3bbcebd
to
d5bd148
Compare
"AA" should not count as a child layer just because of its length, instead look for numbers in the assigned layer
aa009c1
to
7e6d924
Compare
@pytest.mark.skip(reason="filenames changed") | ||
@pytest.mark.remote_data | ||
@pytest.mark.filterwarnings(r"ignore::astropy.wcs.wcs.FITSFixedWarning") | ||
@pytest.mark.parametrize( | ||
"uri, expected_helper", example_uri_helper | ||
) | ||
def test_auto_config_detection(uri, expected_helper): | ||
url = f'https://mast.stsci.edu/api/v0.1/Download/file/?uri={uri}' | ||
fn = download_file(url, timeout=100) | ||
helper_name, hdul = identify_helper(fn) | ||
hdul.close() | ||
assert helper_name == expected_helper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to test_config_detection.py
def test_data_menu_toggles(specviz_helper, spectrum1d): | ||
# load 2 data entries | ||
specviz_helper.load_data(spectrum1d, data_label="test") | ||
app = specviz_helper.app | ||
sv = app.get_viewer('spectrum-viewer') | ||
new_spec = specviz_helper.get_spectra(apply_slider_redshift=True)["test"]*0.9 | ||
specviz_helper.load_data(new_spec, data_label="test2") | ||
|
||
# check that both are enabled in the data menu | ||
selected_data_items = app._viewer_item_by_id('specviz-0')['selected_data_items'] | ||
data_ids = list(selected_data_items.keys()) | ||
assert len(data_ids) == 2 | ||
assert np.all([visible == 'visible' for visible in selected_data_items.values()]) | ||
assert len(sv.layers) == 2 | ||
assert np.all([layer.visible for layer in sv.layers]) | ||
|
||
# disable (hide layer) for second entry | ||
app.set_data_visibility('specviz-0', app._get_data_item_by_id(data_ids[-1])['name'], | ||
visible=False) | ||
|
||
selected_data_items = app._viewer_item_by_id('specviz-0')['selected_data_items'] | ||
assert selected_data_items[data_ids[0]] == 'visible' | ||
assert sv.layers[0].visible is True | ||
assert selected_data_items[data_ids[1]] == 'hidden' | ||
assert sv.layers[1].visible is False | ||
|
||
# add a subset and make sure it appears for the first data entry but not the second | ||
specviz_helper.plugins['Subset Tools'].import_region( | ||
SpectralRegion(6000 * spectrum1d.spectral_axis.unit, 6500 * spectrum1d.spectral_axis.unit)) | ||
|
||
assert len(sv.layers) == 4 | ||
assert sv.layers[2].visible is True # subset corresponding to first (visible) data entry | ||
assert sv.layers[3].visible is False # subset corresponding to second (hidden) data entry | ||
|
||
# enable data layer from menu and subset should also become visible | ||
app.set_data_visibility('specviz-0', app._get_data_item_by_id(data_ids[-1])['name'], | ||
visible=True) | ||
assert np.all([layer.visible for layer in sv.layers]) | ||
|
||
# manually hide just the data layer, and the visiblity state in the menu should show as mixed | ||
sv.layers[3].visible = False | ||
|
||
selected_data_items = app._viewer_item_by_id('specviz-0')['selected_data_items'] | ||
assert selected_data_items[data_ids[0]] == 'visible' | ||
assert selected_data_items[data_ids[1]] == 'mixed' | ||
|
||
|
||
def test_visibility_toggle(imviz_helper): | ||
arr = np.arange(4).reshape((2, 2)) | ||
imviz_helper.load_data(arr, data_label='no_results_data') | ||
app = imviz_helper.app | ||
iv = app.get_viewer('imviz-0') | ||
|
||
# regression test for https://github.com/spacetelescope/jdaviz/issues/1723 | ||
# test to make sure plot options (including percentile) stick when toggling visibility | ||
# via the data menu checkboxes | ||
selected_data_items = app._viewer_item_by_id('imviz-0')['selected_data_items'] | ||
data_ids = list(selected_data_items.keys()) | ||
assert selected_data_items[data_ids[0]] == 'visible' | ||
assert iv.layers[0].visible is True | ||
po = imviz_helper.plugins['Plot Options'] | ||
po.stretch_preset = 90 | ||
assert po.stretch_preset.value == 90 | ||
|
||
app.set_data_visibility('imviz-0', app._get_data_item_by_id(data_ids[0])['name'], | ||
visible=False) | ||
|
||
assert iv.layers[0].visible is False | ||
|
||
app.set_data_visibility('imviz-0', app._get_data_item_by_id(data_ids[0])['name'], | ||
visible=True) | ||
assert iv.layers[0].visible is True | ||
assert po.stretch_preset.value == 90 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reimplemented in configs/default/tests/test_data_menu.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the fact that this PR removes more code than it adds. I don't grok all the internals. A quick usability test using Imviz example notebook seems to work fine. Left some general comments. Thanks!
Where is the toggle for single selection on click vs multiselect on click on the new data dropdown? Looks like the latter is the default now. So when I have multiple subset and I want to edit only one of them (in fact, the "edit subset" option is only enabled when one subset is selected), I have to click at least 2x to get to that feature. Was this by design or is there more follow-up work on this?
docs/imviz/displayimages.rst
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first screenshot you see on https://jdaviz--3281.org.readthedocs.build/en/3281/imviz/displayimages.html#displaying-images is now outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for https://jdaviz--3281.org.readthedocs.build/en/3281/specviz/displaying.html etc (probably same image).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the ones I reasonably could. @camipacifici - do you still have the source (with annotations) to update this one? I suppose this probably isn't urgent until the deprecation period is over, but I agree is definitely something we should update at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably do. I'll take a look next week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I wait for this to reach a resolution (one way or another) before I approve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camipacifici said she would do this is a follow-up next week - so only if you think this needs to block merge (but I personally don't think its urgent since the button is still there for the next two releases).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah now that it opens up the new stuff, I am less worried.
@data-item-remove="$emit('data-item-remove', $event)" | ||
@change-reference-data="$emit('change-reference-data', $event)" | ||
></j-viewer-data-select> | ||
<j-tooltip tooltipcontent="data-menu is now opened by clicking on the legend in the top-right of the viewer"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible when people click this anyway, it opens up the data menu at the new location (in addition to this tooltip)? Took me a second to understand what "legend in the top-right of viewer" really mean. I suspect it will be confusing to others as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my original plan, but the way the two are implemented entirely differently (and the way the anchors are done) makes it difficult. If anyone thinks this is worth (significantly) more effort, I can give it another shot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... What about popping up a screenshot or putting a link in the tooltip to point to a screenshot of the new location? If not, it is okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
managed to figure out a way to open the data menu (with the caveat that it will open all instances if there are multiple instances of the app visible)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
p.s. Maybe a different ticket but by now, the fancy videos (https://jdaviz--3281.org.readthedocs.build/en/3281/video_tutorials.html) and GIFs are pretty outdated now. That is especially obvious with this change because data dropdown is fundamental to Jdaviz usage. |
The previous menu's single vs multiselect was for the visibilities (there was no analog for selecting to modify). But either way, this follows (afaik) the current design plan pending user feedback. There is technically an API switch for single vs multi select for the layer selection - we could for now implement the ability to use that mixed with the UI clicks and then it would just be a matter of adding a UI-toggle if we ever want it. |
@pllim - the latest commit enables support for single-select mode if/when switched from the API ( |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3281 +/- ##
==========================================
- Coverage 88.13% 88.02% -0.12%
==========================================
Files 127 127
Lines 19698 19717 +19
==========================================
- Hits 17361 17355 -6
- Misses 2337 2362 +25 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes look good, added/changed descriptions read through well, and remove and added functionality works great!
Description
This pull request removes the old data-menu and exposes the new data-menu. This keeps the old icon/button, but only shows a tooltip pointing to the new data-menu. Old API access is deprecated and points to alternatives in the new data menu API.
Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.